feat(auth): add backup restore manager#76
feat(auth): add backup restore manager#76ndycode wants to merge 50 commits intogit-plan/01-reset-safetyfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughthis pr adds named-backup restore support: new storage APIs (create/list/assess/restore), cli/ui wiring for a Changes
Sequence diagramsequenceDiagram
participant user as User
participant auth as Auth Menu
participant mgr as BackupRestoreManager
participant storage as Storage
participant fs as File System
rect rgba(200,200,255,0.5)
user->>auth: select "restore from backup"
auth->>mgr: runBackupRestoreManager()
end
rect rgba(200,255,200,0.5)
mgr->>storage: listNamedBackups()
storage->>fs: read backups dir
fs-->>storage: backup entries
storage-->>mgr: NamedBackupMetadata[]
end
rect rgba(255,230,200,0.5)
mgr->>storage: assessNamedBackupRestore(name) (batched)
storage->>fs: read backup file(s)
fs-->>storage: backup content
storage-->>mgr: BackupRestoreAssessment[]
mgr->>user: display menu with hints
user->>mgr: choose backup
mgr->>storage: reassessNamedBackupRestore(name)
mgr->>user: prompt confirm()
user->>mgr: confirm
mgr->>storage: restoreNamedBackup(name)
storage->>fs: merge/import accounts
fs-->>storage: write results
storage-->>mgr: {imported, skipped, total, changed}
mgr->>user: show result / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes reviewable concerns
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:316">
P2: Use retry-based cleanup instead of bare `fs.rm` in test teardown to avoid Windows EBUSY/EPERM/ENOTEMPTY flakiness.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:475">
P2: Backup name normalization in path resolution breaks restore for listed backup files whose filenames contain spaces or other non-normalized characters.</violation>
</file>
<file name="test/codex-manager-cli.test.ts">
<violation number="1" location="test/codex-manager-cli.test.ts:1609">
P3: `vi.doMock` here is never unmocked. `vi.resetModules()` does not clear the mock registry, so this confirm mock will leak into later tests and can change their behavior. Add a `vi.doUnmock`/`vi.unmock` cleanup (e.g., in `afterEach`) to keep tests isolated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:132">
P2: Replace the bare `fs.rm` in test cleanup with retry/backoff to avoid Windows CI flakes on transient file locks.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:476">
P2: `resolveNamedBackupPath` allows path traversal via unsanitized backup names before directory confinement.</violation>
</file>
<file name="lib/cli.ts">
<violation number="1" location="lib/cli.ts:283">
P2: `restore-backup` is wired only in the TUI path; the fallback login prompt cannot select this new mode.</violation>
</file>
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4171">
P2: Handle restoreNamedBackup errors so a missing/invalid backup or limit breach doesn’t crash the auth menu.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/cli.ts (1)
49-61:⚠️ Potential issue | 🟠 Majorthe new mode is still unreachable in the readline fallback menu.
this only wires
restore-backupthrough the tty path.promptLoginModeFallback()inlib/cli.ts:174-243and its prompt copy inlib/ui/copy.ts:135-137still never advertise or parse the mode, so users in fallback environments cannot reach the restore flow even thoughrunAuthLogin()handles it atlib/codex-manager.ts:3909-3919. please add a vitest for the fallback branch.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 291-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/cli.ts` around lines 49 - 61, The fallback readline menu never exposes or parses the new "restore-backup" LoginMode, so add it to promptLoginModeFallback() (ensure the option label and value include "restore-backup") and update the fallback prompt text in lib/ui/copy.ts to advertise the restore flow (matching how runAuthLogin() in lib/codex-manager.ts handles "restore-backup"); add a vitest that exercises the fallback branch to assert the prompt lists and correctly parses "restore-backup", and update any affected tests to reference the new behavior; while modifying prompts/tests, verify any updated auth rotation or queue logic touched by this change properly retries or surfaces EBUSY/429 errors per existing concurrency and Windows IO guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/storage-paths.md`:
- Line 25: Update docs/reference/storage-paths.md to document the restore entry
point and user workflow for the new backup paths: add a short step-by-step
showing "codex auth login" → select recovery → choose 'restore from backup' and
point readers to the interactive restore implementation in lib/codex-manager.ts
(see the interactive flow around the blocks at ~3909-3919 and the restore logic
at ~4211-4291) so users know how to consume files under
~/.codex/multi-auth/backups/<name>.json; also add or link an upgrade note that
mentions the changed CLI workflow and any new npm scripts introduced so README,
SECURITY, and docs remain consistent with the shipped flags and behavior.
In `@lib/codex-manager.ts`:
- Around line 3909-3920: The recovery menu (restore-backup) is never reachable
because runAuthLogin() only enters the menu loop when
existingStorage.accounts.length > 0; hoist the recovery entry so
runBackupRestoreManager(...) can be invoked even when existingStorage.accounts
is empty: move the restore-backup handling (call to runBackupRestoreManager and
its try/catch) to execute before the non-empty accounts guard in runAuthLogin,
ensuring the menu can present the recovery path on fresh/reset/missing storage;
add a vitest that simulates zero-account storage and asserts the restore flow is
offered and that runBackupRestoreManager is called; update any affected tests
referenced in the auth/login suite and ensure new queue logic (if added) handles
EBUSY and 429 retries per concurrency guidelines.
In `@lib/storage.ts`:
- Around line 1712-1728: loadBackupCandidate currently converts any read error
into a permanent invalid-backup result; change it to detect transient Windows/IO
errors (EPERM, EBUSY, EAGAIN) and retry the read (call to loadAccountsFromPath)
with an exponential/backoff loop (same retry envelope used elsewhere in this
module) before returning an error, and apply the same retry logic to the paired
fs.stat call in this file that checks backup file metadata; add a vitest that
simulates a transient busy/EPERM/EBUSY/EAGAIN failure on the first N attempts
and succeeds thereafter to assert the retry path is exercised and cite/update
the affected tests.
- Around line 1580-1609: The function listNamedBackups currently swallows all
readdir errors except ENOENT by logging and returning an empty array; change the
catch in listNamedBackups so that only ENOENT returns [] while any other error
is re-thrown (preserve the original error object) so callers like
runBackupRestoreManager surface real filesystem errors (EPERM/EBUSY). Update
tests: add a vitest that simulates an unreadable backup directory (make readdir
throw EPERM/EBUSY) and assert that listNamedBackups rejects/throws rather than
returning an empty array. Keep references to getNamedBackupRoot/getStoragePath
for locating the path and ensure loadBackupCandidate/buildNamedBackupMetadata
logic is untouched.
In `@test/codex-manager-cli.test.ts`:
- Around line 2365-2428: Add a regression test that covers the confirm=false
path so restore is blocked: duplicate the existing "restores a named backup..."
test setup (use setInteractiveTTY, mock loadAccountsMock, listNamedBackupsMock,
assessNamedBackupRestoreMock, promptLoginModeMock and selectMock) but make
confirmMock resolve to false and then call
runCodexMultiAuthCli(["auth","login"]); assert exitCode is non-zero (or
indicates cancellation) and that restoreNamedBackupMock was not called; keep
other expectations similar (list/assess called once, select used) to ensure the
safety gate implemented by confirmMock is exercised.
In `@test/storage.test.ts`:
- Around line 1111-1185: The tests cover happy paths but lack regression cases
that ensure restoreNamedBackup and assessNamedBackupRestore properly surface
failures for invalid or missing backup payloads; add deterministic vitest tests
in test/storage.test.ts that (1) create a named backup file with invalid JSON
and assert assessNamedBackupRestore(...) and restoreNamedBackup(...) both
reject/throw, (2) create a backup JSON missing required fields (e.g., no
"accounts") and assert both functions reject with a clear error, and (3) assert
behavior is consistent when a backup file is empty or truncated; use the same
helper functions (createNamedBackup, listNamedBackups, assessNamedBackupRestore,
restoreNamedBackup, clearAccounts, loadAccounts) and filesystem writes to inject
the malformed payloads so the tests reproduce the error-path regressions
deterministically under vitest.
---
Outside diff comments:
In `@lib/cli.ts`:
- Around line 49-61: The fallback readline menu never exposes or parses the new
"restore-backup" LoginMode, so add it to promptLoginModeFallback() (ensure the
option label and value include "restore-backup") and update the fallback prompt
text in lib/ui/copy.ts to advertise the restore flow (matching how
runAuthLogin() in lib/codex-manager.ts handles "restore-backup"); add a vitest
that exercises the fallback branch to assert the prompt lists and correctly
parses "restore-backup", and update any affected tests to reference the new
behavior; while modifying prompts/tests, verify any updated auth rotation or
queue logic touched by this change properly retries or surfaces EBUSY/429 errors
per existing concurrency and Windows IO guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8fa6ad5-4c6c-4110-af7b-d6c8d1c2f62c
📒 Files selected for processing (8)
docs/reference/storage-paths.mdlib/cli.tslib/codex-manager.tslib/storage.tslib/ui/auth-menu.tslib/ui/copy.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/ui/auth-menu.tslib/codex-manager.tslib/ui/copy.tslib/storage.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/storage-paths.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (3)
test/storage.test.ts (1)
333-336: good windows-safe teardown change.line 335 in
test/storage.test.ts:335usesremoveWithRetry, which reduces eperm/ebusy teardown flakes on windows.test/codex-manager-cli.test.ts (2)
506-539: good deterministic mock reset/default setup.line 506 through line 539 in
test/codex-manager-cli.test.tscorrectly resets and reseeds new restore-related mocks, which prevents cross-test state bleed.
2430-2500: good restore-failure recovery coverage.line 2430 through line 2500 in
test/codex-manager-cli.test.tsverifies restore failure is surfaced and the login flow continues, which is the right user-safety behavior.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:1208">
P3: Duplicate test case: this restore-after-deletion scenario is already covered earlier in the same file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@cubic-dev-ai review this PR |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:1351">
P2: Use `removeWithRetry` instead of bare `fs.rm` to avoid transient Windows file-lock failures in tests.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:1807">
P2: Do not swallow non-ENOENT errors when scanning named backups; rethrow them so restore reports the real filesystem failure instead of a misleading “file not found.”</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@cubic-dev-ai review this PR |
|
@coderabbitai full review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
✅ Actions performedFull review triggered. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
test/codex-manager-cli.test.ts (1)
3468-3506:⚠️ Potential issue | 🟠 Majorepoch hint regression is asserted in the wrong direction.
the test name says epoch timestamps are shown, but the assertion expects
"updated "to be absent. this weakens the epoch-rendering contract and can hide regressions. align either the assertion or the test name.test/codex-manager-cli.test.ts:3468-3506,lib/codex-manager.ts:4258.minimal fix if epoch should be rendered
- expect(backupItems?.[0]?.hint).not.toContain("updated "); + expect(backupItems?.[0]?.hint).toContain("updated ");As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 3468 - 3506, The test "shows epoch backup timestamps in restore hints" currently asserts the opposite by checking expect(backupItems?.[0]?.hint).not.toContain("updated "), so update the test assertion to match the test name/intent: change that line to expect(backupItems?.[0]?.hint).toContain("updated ") (or otherwise assert the epoch-rendered timestamp substring you want) in the spec that calls runCodexMultiAuthCli and inspects selectMock.mock.calls (test/codex-manager-cli.test.ts), so the test verifies the epoch timestamp is rendered when assessNamedBackupRestoreMock returns an assessment; keep the rest of the setup (listNamedBackupsMock, promptLoginModeMock, selectMock) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 4361-4365: Replace the direct call to
resolveNamedBackupRestorePath + importAccounts (the block using
latestAssessment.backup.name, resolveNamedBackupRestorePath, and importAccounts)
with the storage-level named-backup restore helper used earlier in the confirm
flow (i.e., call the same storage restore wrapper that produces a safe,
exclusive restore path instead of resolving and opening the file directly), so
the final import uses the storage restore path and its concurrency/lock
handling; ensure the new path handles EBUSY/EPERM and HTTP 429 retry semantics,
preserves auth-rotation (do not leak tokens/emails in logs), and add two vitest
regressions: one in test/storage.test.ts and one in
test/codex-manager-cli.test.ts that simulate "backup removed or locked after
confirm" to assert the code retries/fails safely and logs non-sensitive
diagnostics.
- Around line 4222-4224: Add a direct dispatch for the new restore-backup CLI
command and a unit test: in the runCodexMultiAuthCli function add a branch that
recognizes the ["auth","restore-backup"] args and calls
runBackupRestoreManager(displaySettings) (preserving existing interactive flow),
update printUsage to include the "auth restore-backup" entry so it appears in
help output, and add a vitest in test/codex-manager-cli.test.ts that calls
runCodexMultiAuthCli(["auth","restore-backup"]) (mock or spy
runBackupRestoreManager as needed) to assert the new path is exercised.
In `@lib/storage.ts`:
- Around line 2137-2169: Add a vitest in test/storage.test.ts that exercises
resolveNamedBackupRestorePath's fallback branch by simulating a plain filename
that causes buildNamedBackupPath to throw while findExistingNamedBackupPath
returns null; specifically, call resolveNamedBackupRestorePath with a name that
is a basename (no separators, no "..") whose baseName does not match
/^[A-Za-z0-9_-]+$/ so the catch path executes and the function should throw an
Error whose message equals the “Import file not found: …” string produced by
resolvePath(join(backupRoot, requestedWithExtension)). Mock or stub
buildNamedBackupPath to throw, stub findExistingNamedBackupPath to return null,
and ensure getNamedBackupRoot/getStoragePath produce a stable known backupRoot
(account for Windows path sep) and assert the thrown error message matches the
exact resolved path; add the test as a regression for race/lock scenarios around
resolveNamedBackupRestorePath, buildNamedBackupPath, and
assertNamedBackupRestorePath.
In `@test/codex-manager-cli.test.ts`:
- Around line 2659-2732: Add a deterministic regression test that simulates the
backup disappearing after selection by making
resolveNamedBackupRestorePathMock.reject with an ENOENT error (e.g., new Error
with code "ENOENT") when the user selects the named backup; then assert the CLI
returns to the login menu (promptLoginModeMock called again), that
importAccountsMock and assessNamedBackupRestoreMock were not called for the
final resolve, and that a restore-failure message containing "ENOENT" or
"Restore failed" was logged; use the existing test scaffolding (selectMock,
promptLoginModeMock, runCodexMultiAuthCli) and vitest mocks so the test remains
deterministic.
In `@test/storage.test.ts`:
- Around line 2430-2448: The test asserts readdirSpy was called 7 times (1
initial + 6 retries) which is unclear; update the assertion to derive the
expected call count from the retry policy (e.g., use the
retryTransientFilesystemOperation retry constant or export a MAX_RETRIES value)
or add a one-line comment above the expectation explaining "1 initial attempt +
N retries" so readers can see why 7 is expected; locate the assertion around
restoreNamedBackup and readdirSpy in the test and either compute expectedCalls =
1 + MAX_RETRIES for the toHaveBeenCalledTimes check or add the clarifying
comment referencing retryTransientFilesystemOperation.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 3468-3506: The test "shows epoch backup timestamps in restore
hints" currently asserts the opposite by checking
expect(backupItems?.[0]?.hint).not.toContain("updated "), so update the test
assertion to match the test name/intent: change that line to
expect(backupItems?.[0]?.hint).toContain("updated ") (or otherwise assert the
epoch-rendered timestamp substring you want) in the spec that calls
runCodexMultiAuthCli and inspects selectMock.mock.calls
(test/codex-manager-cli.test.ts), so the test verifies the epoch timestamp is
rendered when assessNamedBackupRestoreMock returns an assessment; keep the rest
of the setup (listNamedBackupsMock, promptLoginModeMock, selectMock) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8e11074-2d65-440a-bea3-4661b061dd6c
📒 Files selected for processing (9)
docs/reference/storage-paths.mdlib/cli.tslib/codex-manager.tslib/storage.tslib/ui/auth-menu.tslib/ui/copy.tstest/cli.test.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/ui/copy.tslib/codex-manager.tslib/ui/auth-menu.tslib/storage.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/codex-manager-cli.test.tstest/cli.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/storage-paths.md
🔇 Additional comments (22)
test/cli.test.ts (1)
707-729: good restore alias coverage.test/cli.test.ts:707-729 now locks all fallback aliases (
u,restore,backup,restore-backup) tomode: "restore-backup". this closes the mapping gap cleanly.lib/storage.ts (1)
1644-1762: retry envelope and bounded listing concurrency look solid.lib/storage.ts:1644-1762 (line 1650 and line 1739) correctly combines retryable windows filesystem handling with bounded batch processing, and it preserves hard failures outside
ENOENT.lib/cli.ts (1)
60-60: restore-backup mode wiring is consistent across fallback and tty flows.lib/cli.ts:237-244 and lib/cli.ts:299-300 map the same restore action in both input paths, and test/cli.test.ts:707-729 covers the fallback aliases.
Also applies to: 237-244, 299-300
docs/reference/storage-paths.md (1)
25-25: docs now describe the shipped restore entry point clearly.docs/reference/storage-paths.md:105-111 and docs/reference/storage-paths.md:127 align with the interactive
codex auth loginrecovery flow and named-backup paths.Also applies to: 60-60, 105-111, 127-127
lib/ui/auth-menu.ts (1)
67-67: menu action and recovery section integration look correct.lib/ui/auth-menu.ts:67, lib/ui/auth-menu.ts:521-522, and lib/ui/auth-menu.ts:660-670 stay type-safe and keep focus navigation consistent with the new restore action.
Also applies to: 521-522, 660-670
lib/ui/copy.ts (1)
17-37: restore copy changes are aligned with the new flow.lib/ui/copy.ts:17-37 and lib/ui/copy.ts:155-156 add the recovery wording and fallback prompt updates needed for restore-backup without reintroducing the old refreshed-count ambiguity.
Also applies to: 155-156
test/storage.test.ts (11)
338-341: good use ofremoveWithRetryfor windows compatibility in teardown.the switch from
fs.rmtoremoveWithRetryattest/storage.test.ts:340addresses ebusy/eperm flakiness on windows during test cleanup. this follows the pattern established intest/helpers/remove-with-retry.tsand keeps the import/export test block deterministic.
475-511: scoped writefile spy assertions correctly filter by storage path.the filtering logic at
test/storage.test.ts:501-507now checkstarget.startsWith(\${testStoragePath}.`)` to include wal/bak variants without catching unrelated writes. this addresses the previous concern about overly broad assertions and keeps the test deterministic.
1584-1593: fallback path for missing named backups now covered.
test/storage.test.ts:1584-1593exercisesresolveNamedBackupRestorePathwhen the backup file doesn't exist, verifying it returns a contained path viabuildNamedBackupPathand thatimportAccountscorrectly rejects with "import file not found". this addresses the greptile-flagged coverage gap for the windows concurrency edge case where a backup disappears between list and restore.
1985-2039: toctou guards with atomicity assertions look solid.tests at
test/storage.test.ts:1985-2011andtest/storage.test.ts:2013-2039verify that when a named backup is deleted or corrupted after assessment,restoreNamedBackupthrows and active storage remains untouched. the assertionsexpect((await loadAccounts())?.accounts ?? []).toHaveLength(0)pin restore atomicity as required.
2041-2051: path traversal error matching is now specific.the regex at
test/storage.test.ts:2045andtest/storage.test.ts:2048uses/must not contain path separators/iwhich matches the actual error text fromlib/storage.ts. this addresses the past review comment about overly broad alternations.
2547-2596: transient eperm retry path on win32 now covered.
test/storage.test.ts:2547-2596mocks platform aswin32and injects a single eperm failure duringlistNamedBackups(), asserting the operation succeeds after retry. this addresses the past review comment about missing eperm retry coverage for the named-backup path.
3049-3101: concurrency workload scales with exported limit.
test/storage.test.ts:3051usesNAMED_BACKUP_LIST_CONCURRENCY + 4for total backup count, ensuring the test always exceeds the limiter threshold regardless of the configured concurrency value. the assertion attest/storage.test.ts:3095-3097correctly references the constant instead of a magic number.
2730-2766: manual backup deletion atomicity regression is thorough.
test/storage.test.ts:2730-2766captures storage state before restore, deletes the backup, and asserts both the throw and thatloadAccounts()returns the unchanged pre-restore state. this pins restore atomicity for the manual-backup scenario as flagged in past review.
3103-3172: concurrent restore serialization test is well-designed.
test/storage.test.ts:3103-3172creates a near-limit state with two eligible backups, then racesrestoreNamedBackupon both. asserting exactly one succeeds and one fails with "import would exceed maximum" verifies the transaction lock serializes restores correctly. good coverage for the concurrency bug regression case.
2115-2170: symlink/junction traversal hardening is pinned.tests at
test/storage.test.ts:2115-2170create symlinks (junctions on win32) pointing outside the backup root and verifyassertNamedBackupRestorePaththrows "escapes backup directory". this covers the windows junction edge case where the selected name doesn't contain..but resolves outside the root.
2336-2390: listing ignores symlink entries and assessment rejects them explicitly.
test/storage.test.ts:2336-2390mocks areaddirentry withisSymbolicLink() => true, verifieslistNamedBackups()returns empty, and assertsassessNamedBackupRestore/restoreNamedBackupthrow "not a regular backup file". this closes the traversal hardening gap for symlinked entries inside the backups directory.test/codex-manager-cli.test.ts (4)
517-556: good baseline reset for the new restore mocks.this setup keeps restore-path tests isolated and deterministic across cases.
test/codex-manager-cli.test.ts:517-556.
2592-2657: good safety-gate regression for confirm=false.this explicitly locks the user-confirmation guard and verifies the login menu loop continues.
test/codex-manager-cli.test.ts:2592-2657.
3022-3097: good bounded-concurrency regression for backup assessment.the deferred gate plus
maxInFlightassertion directly protectsnamed_backup_assess_concurrencybehavior.test/codex-manager-cli.test.ts:3022-3097.
4901-4989: good race-ordering check before restore menu work starts.the invocation-order assertion clearly verifies quota refresh completion precedes backup listing.
test/codex-manager-cli.test.ts:4901-4989.lib/codex-manager.ts (1)
3829-3885: nice recovery loop and quota-refresh serialization.loading
currentStorageon every menu iteration atlib/codex-manager.ts:3830-3834keeps the restore path reachable even when storage is empty, and awaitingpendingMenuQuotaRefreshbefore the quota-touching actions atlib/codex-manager.ts:3875-3885avoids reintroducing the cache-writer race. good change. the new coverage intest/codex-manager-cli.test.ts:1is the right place to keep this pinned.
There was a problem hiding this comment.
Pull request overview
Adds a named-backup restore/recovery flow to codex auth login and a direct codex auth restore-backup command, including backup listing, assessment, and restore logic with containment/symlink guards and more robust import/restore behavior.
Changes:
- Introduces named-backup listing/assessment/restore APIs in storage, including transient filesystem retry and containment validation.
- Adds interactive “Recovery → Restore From Backup” menu + a
restore-backupCLI entrypoint and confirmation flow. - Expands test coverage for imports (no-op/metadata refresh/dedup/limit) and backup restore manager behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/storage.ts |
Implements named-backup metadata, listing, assessment, restore, containment checks, transient FS retries, and import no-op detection. |
lib/codex-manager.ts |
Adds interactive restore manager flow and codex auth restore-backup command wiring. |
lib/cli.ts |
Adds restore-backup login mode + fallback aliases. |
lib/ui/auth-menu.ts |
Adds “Recovery” section and “Restore From Backup” action to the auth menu. |
lib/ui/copy.ts |
Adds UI copy for restore confirmations and success messaging; updates fallback prompt text. |
docs/reference/storage-paths.md |
Documents named backup paths and the restore workflow. |
test/storage.test.ts |
Adds extensive coverage for import no-op behavior, named backups, containment guards, and transient FS handling. |
test/codex-manager-cli.test.ts |
Adds restore-backup CLI/menu tests + error-handling and concurrency/ordering expectations. |
test/cli.test.ts |
Tests restore-backup fallback alias inputs. |
Comments suppressed due to low confidence (1)
lib/storage.ts:3108
- importAccounts()
now persists a deduplicated account list but keepsactiveIndexunchanged. If the existing storage contained duplicates andexistingActiveIndexpointed to an entry that is removed by deduplication,activeIndexcan become out-of-range in the persisted file. Consider clampingactiveIndex(and any per-family active index fields) to[0, accounts.length - 1]` after deduplication—ideally via a shared helper used elsewhere for index normalization.
const newStorage: AccountStorageV3 = {
version: 3,
accounts: deduplicatedAccounts,
activeIndex: existingActiveIndex,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function importAccounts( | ||
| filePath: string, | ||
| ): Promise<{ imported: number; total: number; skipped: number }> { | ||
| ): Promise<ImportAccountsResult> { |
| lstat(path: string) { | ||
| return lstatSync(path); | ||
| }, | ||
| realpath(path: string) { | ||
| return realpathSync.native(path); |
lib/codex-manager.ts
Outdated
| console.error( | ||
| `Could not read backup directory: ${ | ||
| collapseWhitespace(message) || "unknown error" | ||
| }`, | ||
| ); |
|
Follow-up pushed on the current PR head ( Changes in this pass:
@greptile review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4474">
P2: CLI `restore-backup` command falsely exits with a success code (0) when the restore fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/storage.test.ts (1)
1587-1596:⚠️ Potential issue | 🟠 Majoradd a direct restore regression for the fallback-miss path.
at
test/storage.test.ts:1587(Line [1587]), this only pinsresolveNamedBackupRestorePath(...)+importAccounts(...). it does not pinrestoreNamedBackup(...)when the requested name is absent from listings and the fallback path is used. please add a deterministic case that callsrestoreNamedBackup(" missing-backup "), asserts/Import file not found/, and confirms active storage is unchanged.proposed test addition
+ it("throws file-not-found for never-present named backups and leaves storage unchanged", async () => { + await clearAccounts(); + const before = await loadAccounts(); + await expect(restoreNamedBackup(" missing-backup ")).rejects.toThrow( + /Import file not found/, + ); + expect(await loadAccounts()).toEqual(before); + });As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 1587 - 1596, Add a deterministic regression test that calls restoreNamedBackup(" missing-backup "), expects it to reject with /Import file not found/, and verifies active storage was not modified; specifically, after calling restoreNamedBackup(" missing-backup ") assert it rejects toThrow(/Import file not found/) and then confirm active storage remains the same (e.g., compare a pre-call snapshot obtained via the same accessor used elsewhere in tests such as listing or reading current accounts) — use resolveNamedBackupRestorePath/requestedName trimming behavior and the existing importAccounts/buildNamedBackupPath helpers to mirror the fallback-miss scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 4477-4488: The restore-backup branch fails to clear a previously
set storage scope; call setStoragePath(null) before invoking
runBackupRestoreManager in the command === "restore-backup" block so lingering
storage scope won't point the restore at the wrong directory/account, and add a
vitest in test/codex-manager-cli.test.ts that seeds a scoped storage path then
invokes runCodexMultiAuthCli(["auth","restore-backup"]) to assert the restore
uses the cleared scope; reference runBackupRestoreManager, setStoragePath, and
runCodexMultiAuthCli when locating and updating the code and test.
- Around line 4255-4338: The loop that calls assessNamedBackupRestore can skip
all backups and leave assessments empty, which yields a back-only picker and
allows the restore command to incorrectly return success; after the
Promise.allSettled processing and before building items, detect if
assessments.length === 0, consolidate the non-containment rejection reasons from
settledAssessments (use collapseWhitespace to normalize messages and fall back
to a single "all assessments failed" message), log or surface that consolidated
error, and return false to indicate failure; reference the variables/idents
assessNamedBackupRestore, settledAssessments, isNamedBackupContainmentError,
collapseWhitespace, assessments, select and backupDir when locating where to add
this check, and update the corresponding test test/codex-manager-cli.test.ts to
cover the all-failed case.
In `@lib/storage.ts`:
- Around line 3079-3088: The code does a check-then-read race: after using
existsSync(resolvedPath) the file may be deleted before
retryTransientFilesystemOperation(() => fs.readFile(resolvedPath, "utf-8")),
causing a raw ENOENT to bubble up; wrap the read in a try/catch inside the retry
(or immediately around the awaited call) and if you see an ENOENT map/throw a
new Error(`Import file not found: ${resolvedPath}`) so the higher-level import
contract is preserved; update/ add vitest cases in test/storage.test.ts and
test/codex-manager-cli.test.ts that delete the file between existsSync and read
to assert the translated error, and ensure you reference resolvedPath,
existsSync, retryTransientFilesystemOperation, and fs.readFile when editing.
---
Duplicate comments:
In `@test/storage.test.ts`:
- Around line 1587-1596: Add a deterministic regression test that calls
restoreNamedBackup(" missing-backup "), expects it to reject with /Import file
not found/, and verifies active storage was not modified; specifically, after
calling restoreNamedBackup(" missing-backup ") assert it rejects
toThrow(/Import file not found/) and then confirm active storage remains the
same (e.g., compare a pre-call snapshot obtained via the same accessor used
elsewhere in tests such as listing or reading current accounts) — use
resolveNamedBackupRestorePath/requestedName trimming behavior and the existing
importAccounts/buildNamedBackupPath helpers to mirror the fallback-miss
scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9d0c172-8bd4-4e83-b086-0379a3875eee
📒 Files selected for processing (7)
docs/reference/commands.mddocs/reference/public-api.mddocs/reference/storage-paths.mdlib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Greptile Review
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/storage-paths.mddocs/reference/commands.mddocs/reference/public-api.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-cli.test.tstest/storage.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/storage.ts
🔇 Additional comments (12)
test/codex-manager-cli.test.ts (11)
558-562: mock correctly chains internal storage calls perlib/storage.ts:1889-1901.the
restoreAssessedNamedBackupMockimplementation at lines 558-562 mirrors the realrestoreAssessedNamedBackupbehavior: it callsresolveNamedBackupRestorePathfirst, thenimportAccounts. this ensures tests exercising path validation failures (liketest/codex-manager-cli.test.ts:2630) surface correctly whenresolveNamedBackupRestorePathMockrejects.one note: the mock doesn't check
eligibleForRestorelike the real implementation does atlib/storage.ts:1892-1896. this is fine since production code (lib/codex-manager.ts:4307per pr context) checks eligibility before callingrestoreAssessedNamedBackup, and tests at lines 3477-3572 verify this flow.
3216-3291: solid concurrency regression test for assessment cap.this test properly validates the
NAMED_BACKUP_ASSESS_CONCURRENCYbound by:
- importing the actual constant at
test/codex-manager-cli.test.ts:3219-3222viavi.importActual- using deferred promises with microtask scheduling at lines 3241-3257 to control release timing
- tracking
maxInFlightand asserting it stays within bounds at lines 3288-3290the deferred-gate pattern ensures deterministic timing without relying on sleep or wall-clock delays.
3574-3660: good regression for reassessment EBUSY rejection path.test at
test/codex-manager-cli.test.ts:3574-3660covers the case where re-assessment fails withEBUSYbefore confirmation. this exercises the error path atlib/codex-manager.ts:4307per pr context and verifies:
confirmMocknot called (line 3652)importAccountsMocknot called (line 3653)- error message logged (line 3654-3656)
- login menu re-entered (line 3633)
this addresses the past review comment about reassessment rejection regression.
5095-5183: good quota refresh ordering regression for backup restore.this test at
test/codex-manager-cli.test.ts:5095-5183verifies that in-flight menu quota refresh completes beforelistNamedBackupsis called. uses deferred promises at lines 5142-5153 to gate the fetch, then asserts ordering at lines 5176-5179 viainvocationCallOrder. this covers a potential race where backup restore could start before quota cache is persisted.
3702-3794: deterministic timestamp formatting test.the test at
test/codex-manager-cli.test.ts:3702-3794fixes timestamp toDate.UTC(2026, 0, 10, 12, 0, 0)viavi.spyOn(Date, "now")at line 3706. this avoids locale-dependent failures that would occur withtoLocaleDateString(). the assertions at lines 3787-3790 check relative time labels ("today", "yesterday", "3d ago") rather than formatted date strings.this addresses the past review comment about avoiding exact localized date string matching.
2730-2795: decline path regression covers menu re-entry.test at
test/codex-manager-cli.test.ts:2730-2795setsconfirmMock.mockResolvedValueOnce(false)at line 2773 and asserts:
promptLoginModeMockcalled twice (line 2793) - verifies loop back to menuimportAccountsMocknot called (line 2794) - verifies no restore attemptedthis addresses the past review comment about asserting the decline path loops back to login.
3011-3041: manager-level EPERM regression for backup discovery.test at
test/codex-manager-cli.test.ts:3011-3041usesmakeErrnoError("EPERM...", "EPERM")at lines 3014-3017 to simulate windows-style permission failure onlistNamedBackups. verifies:
assessNamedBackupRestoreMocknot called (line 3030)- error message contains EPERM (lines 3033-3037)
- login menu re-entered (line 3029)
this addresses the past review comment about manager-level EPERM regression for backup discovery.
2389-2455: login recovery restore flow well-tested.test at
test/codex-manager-cli.test.ts:2389-2455exercises the happy path for restore via login recovery menu:
listNamedBackupsMockcalled (line 2441)assessNamedBackupRestoreMockcalled with correct args (lines 2442-2449)confirmMockcalled once (line 2450)importAccountsMockcalled with resolved path (lines 2451-2453)setCodexCliActiveSelectionMockcalled to sync active account (line 2454)assertions cover the full restore-and-sync flow.
2587-2653: path containment regression covers TOCTOU scenario.test at
test/codex-manager-cli.test.ts:2587-2653simulates backup root changing between assessment and final import by makingresolveNamedBackupRestorePathMockreject at line 2630-2632 with "Backup path escapes backup directory". verifies:
confirmMockwas called (line 2644) - user already confirmed before path revalidationimportAccountsMocknot called (line 2645) - blocked by path validation- error logged (lines 2647-2649)
- login menu re-entered (line 2646)
this covers the TOCTOU re-assessment guard per pr objectives.
3148-3214: partial assessment failure regression with warn logging.test at
test/codex-manager-cli.test.ts:3148-3214covers mixed assessment results where one backup fails and one succeeds:
assessNamedBackupRestoreMockthrows for "broken-backup" at lines 3182-3184- healthy backup remains selectable (lines 3191-3194)
- warning logged for skipped backup (lines 3206-3210)
- restore proceeds for healthy backup (lines 3203-3205)
good coverage for partial failure resilience in the restore menu.
1-10: mock scaffold well-structured for named-backup flows.the mock additions provide comprehensive coverage:
MOCK_BACKUP_DIRandmockBackupPathat lines 5-9 create consistent test paths- storage mocks at lines 120-125 intercept all named-backup apis
confirmMockat lines 209-213 enables confirmation flow testingbeforeEachsetup at lines 519-563 provides sensible defaults while allowing per-test overridesthe layered mock approach keeps individual tests focused on their specific scenarios.
Also applies to: 17-22, 107-129, 209-214, 519-563
test/storage.test.ts (1)
3116-3168: nice limiter regression coverage here.
test/storage.test.ts:3116usesNAMED_BACKUP_LIST_CONCURRENCYto scale workload and asserts peak read concurrency stays bounded. this is a strong, deterministic vitest regression for the queue-throttling contract.
|
Follow-up pushed on the current PR head ( Changes in this pass:
Validation rerun:
@coderabbitai review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedReview triggered.
|
|
PR body reset to the concise current summary for head @greptile review |
|
✅ Actions performedFull review triggered. |
What
codex auth loginand a directcodex auth restore-backupentrypointENOENTmapping, metadata-refresh, direct-command exit codes, and Windows lock edge cases with targeted vitest regressionsWhy
Risk
Low to medium. Scope stays inside the backup restore flow, storage containment/retry logic, CLI status handling, and targeted tests.
Rollback
Revert the follow-up commits on
git-plan/02-backup-restore-managerif backup restore behavior regresses.Validation
npx eslint lib/storage.ts lib/codex-manager.ts test/storage.test.ts test/codex-manager-cli.test.tsnpx vitest run test/storage.test.ts test/codex-manager-cli.test.ts --maxWorkers=1npm run typecheckblocked in this worktree because@codex-ai/plugin,@codex-ai/plugin/tool, and@codex-ai/sdkare not installed herenpm testblocked for the same missing-package reason in this worktreeDocs
docs/reference/commands.mddocs/reference/storage-paths.mddocs/reference/public-api.mdGovernance
dist/changesef05872